Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmake: Fix TryAppendCXXFlags module #287

Merged
merged 3 commits into from
Jul 26, 2024
Merged

cmake: Fix TryAppendCXXFlags module #287

merged 3 commits into from
Jul 26, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jul 26, 2024

This PR fixes a couple of bugs in the TryAppendCXXFlags module that I noticed during testing on the staging branch:

  1. Fixes the check of multiple flags. As documented, a space must be used as a separator:

    bitcoin/cmake/module/TryAppendCXXFlags.cmake

    Lines 12 to 14 in 56450e5

    Usage examples:
    try_append_cxx_flags("-Wformat -Wformat-security" VAR warn_cxx_flags)

Otherwise, only the first flag is actually being checked.

Here are excerpts from build/CMakeFiles/CMakeConfigureLog.yaml:

    kind: "try_compile-v1"
    backtrace:
      - "/usr/share/cmake-3.27/Modules/Internal/CheckSourceCompiles.cmake:101 (try_compile)"
      - "/usr/share/cmake-3.27/Modules/CheckCXXSourceCompiles.cmake:52 (cmake_check_source_compiles)"
      - "cmake/module/TryAppendCXXFlags.cmake:70 (check_cxx_source_compiles)"
      - "CMakeLists.txt:416 (try_append_cxx_flags)"
    checks:
      - "Performing Test CXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY"
    directories:
      source: "/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-8Q9qSw"
      binary: "/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-8Q9qSw"
    cmakeVariables:
      CMAKE_CXX_FLAGS: ""
      CMAKE_CXX_FLAGS_DEBUG: "-g"
      CMAKE_CXX_LINK_NO_PIE_SUPPORTED: "1"
      CMAKE_EXE_LINKER_FLAGS: ""
      CMAKE_MODULE_PATH: "/home/hebasto/git/bitcoin/cmake/module"
      CMAKE_POSITION_INDEPENDENT_CODE: "ON"
    buildResult:
      variable: "CXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY"
      cached: true
      stdout: |
        Change Dir: '/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-8Q9qSw'
        
        Run Build Command(s): /usr/bin/cmake -E env VERBOSE=1 /usr/bin/gmake -f Makefile cmTC_52a2c/fast
        /usr/bin/gmake  -f CMakeFiles/cmTC_52a2c.dir/build.make CMakeFiles/cmTC_52a2c.dir/build
        gmake[1]: Entering directory '/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-8Q9qSw'
        Building CXX object CMakeFiles/cmTC_52a2c.dir/src.cxx.o
        /usr/bin/c++ -DCXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY  -Wformat -std=c++20 -fPIC -o CMakeFiles/cmTC_52a2c.dir/src.cxx.o -c /home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-8Q9qSw/src.cxx
        Linking CXX static library libcmTC_52a2c.a
        /usr/bin/cmake -P CMakeFiles/cmTC_52a2c.dir/cmake_clean_target.cmake
        /usr/bin/cmake -E cmake_link_script CMakeFiles/cmTC_52a2c.dir/link.txt --verbose=1
        /usr/bin/ar qc libcmTC_52a2c.a CMakeFiles/cmTC_52a2c.dir/src.cxx.o
        /usr/bin/ranlib libcmTC_52a2c.a
        gmake[1]: Leaving directory '/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-8Q9qSw'
        
      exitCode: 0
  • with this PR:
    kind: "try_compile-v1"
    backtrace:
      - "/usr/share/cmake-3.27/Modules/Internal/CheckSourceCompiles.cmake:101 (try_compile)"
      - "/usr/share/cmake-3.27/Modules/CheckCXXSourceCompiles.cmake:52 (cmake_check_source_compiles)"
      - "cmake/module/TryAppendCXXFlags.cmake:70 (check_cxx_source_compiles)"
      - "CMakeLists.txt:416 (try_append_cxx_flags)"
    checks:
      - "Performing Test CXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY"
    directories:
      source: "/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-SK4Pzn"
      binary: "/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-SK4Pzn"
    cmakeVariables:
      CMAKE_CXX_FLAGS: ""
      CMAKE_CXX_FLAGS_DEBUG: "-g"
      CMAKE_CXX_LINK_NO_PIE_SUPPORTED: "1"
      CMAKE_EXE_LINKER_FLAGS: ""
      CMAKE_MODULE_PATH: "/home/hebasto/git/bitcoin/cmake/module"
      CMAKE_POSITION_INDEPENDENT_CODE: "ON"
    buildResult:
      variable: "CXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY"
      cached: true
      stdout: |
        Change Dir: '/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-SK4Pzn'
        
        Run Build Command(s): /usr/bin/cmake -E env VERBOSE=1 /usr/bin/gmake -f Makefile cmTC_ef70a/fast
        /usr/bin/gmake  -f CMakeFiles/cmTC_ef70a.dir/build.make CMakeFiles/cmTC_ef70a.dir/build
        gmake[1]: Entering directory '/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-SK4Pzn'
        Building CXX object CMakeFiles/cmTC_ef70a.dir/src.cxx.o
        /usr/bin/c++ -DCXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY  -Wformat -Wformat-security -Werror -std=c++20 -fPIC -o CMakeFiles/cmTC_ef70a.dir/src.cxx.o -c /home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-SK4Pzn/src.cxx
        Linking CXX static library libcmTC_ef70a.a
        /usr/bin/cmake -P CMakeFiles/cmTC_ef70a.dir/cmake_clean_target.cmake
        /usr/bin/cmake -E cmake_link_script CMakeFiles/cmTC_ef70a.dir/link.txt --verbose=1
        /usr/bin/ar qc libcmTC_ef70a.a CMakeFiles/cmTC_ef70a.dir/src.cxx.o
        /usr/bin/ranlib libcmTC_ef70a.a
        gmake[1]: Leaving directory '/home/hebasto/git/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-SK4Pzn'
        
      exitCode: 0
  1. Avoid skipping checks for linking due to caching of the result variable. The effect can be observed in the configuration output.

@hebasto hebasto added the bug Something isn't working label Jul 26, 2024
@m3dwards
Copy link

ACK 634b3c7

I can replicate the problem without the PR and see that it's fixed with the PR.

cat build/CMakeFiles/CMakeConfigureLog.yaml | grep format-security
        /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DCXX_SUPPORTS__WFORMAT__WFORMAT_SECURITY  -Wformat -Wformat-security -Werror -std=c++20 -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk -fPIC -MD -MT CMakeFiles/cmTC_9f576.dir/src.cxx.o -MF CMakeFiles/cmTC_9f576.dir/src.cxx.o.d -o CMakeFiles/cmTC_9f576.dir/src.cxx.o -c /Users/maxedwards/source/bitcoin/build/CMakeFiles/CMakeScratch/TryCompile-IECvyk/src.cxx

@hebasto hebasto merged commit e4f7e12 into cmake-staging Jul 26, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hebasto @m3dwards